-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port C API to new Context API #2973
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't change the signatures of wasm_global_type
, wasm_global_set
, ... from store
to ctx
since those signatures are given by the headers in the official wasm-c-api and we must preserve them.
You can check the official headers here: https://github.com/WebAssembly/wasm-c-api
I propose to attach the ctx
to the store
itself (when it's created on the c-api) and that way there will be no breaking changes on the API.
Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments over locking the context, but it's intended for almost any usage of the ctx.lock()
in the PR.
In general, we shouldn't rely on locking the context. The Wasm C-API is designed to push that responsibility into the user code, not on the API side
2892: Implement new Context API for Wasmer 3.0 r=epilys a=Amanieu ### Overview This PR reworks the way Wasmer manages objects used by instances, specifically: `Instance`, `Memory`, `Table`, `Function`, `Global` and `ExternRef`. Previously these were tracked using reference counting, but this cannot work fully because Wasm code can manipulate references to functions (and by implication the instance that function is from) using tables. The new approach uses a `Context` type which owns all of the objects listed above: objects are only freed when the `Context` is dropped. The `Instance`, `Memory`, `Table`, `Function`, `Global` and `ExternRef` types are now simply wrappers around an integer which indexes a vector in the `Context`. As a result, any function on those "handle" types now need to take a `&Context` or `&mut Context` as a parameter to access the actual data in the context. This also vastly simplifies Wasmer's thread-safety story: since all mutating operations require a `&mut Context`, no synchronization or internal locks are needed. Additionally, the way host functions are represented is also changed. The `WasmerEnv` trait is removed since it was unergonomic and hard to use correctly. Instead, the state for host functions is stored directly as the `T` in `Context<T>`. This state is initialized when the context is created, and can be accessed through the `data` and `data_mut` methods on `Context<T>`. All host functions receive a `ContextMut<'_, T>` as their first argument, which gives them access to the host state of the context. This is much better than the old approach since it provides mutable access without locks and allows all host functions to share the same state. This design is heavily inspired by [this RFC](https://github.com/bytecodealliance/rfcs/blob/main/accepted/new-api.md) from Wasmtime. ### TODO - [x] #2985 - [x] #2908 - [x] #2973 Upgrade `c-api` to use the new API - [x] Update wasmer-wasi to use the new API - [x] #2909 - [x] #2913 - [x] #2912 - #2911 (Non blocking) - [ ] #2910 Fixes #2893 Fixes #2873 Fixes #2866 Fixes #1840 Fixes #1734 Fixes #1632 Fixes #1522 Fixes #1630 Fixes #2838 Fixes #2856 Fixes #2544 Fixes #2816 Co-authored-by: Amanieu d'Antras <[email protected]> Co-authored-by: ptitSeb <[email protected]> Co-authored-by: Manos Pitsidianakis <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>
Not done: tests